Fix/ns views not cleaned on removal #1740

m-abs · 2019-02-18T16:29:48Z

PR Checklist

The PR title follows our guidelines: https://github.com/NativeScript/NativeScript/blob/master/CONTRIBUTING.md#commit-messages.
There is an issue for the bug/feature this PR is for. To avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it.
You have signed the CLA.
All existing tests are passing: https://github.com/NativeScript/nativescript-angular/blob/master/DevelopmentWorkflow.md#running-the-tests
Tests for the changes are included.

What is the current behavior?

See the description in #1738

What is the new behavior?

ViewUtil.removeChild(parent, child) now removes children recursively to clear references.
TemplatedItemsComponent.ngOnDestroy() empties the _templateMap to remove a circular reference to the TemplatedItemsComponent in the factory functions.
PageRouterOutlet.ngOnDestroy() will now destroy the activated componentRef.
NativeScriptRendererFactory.ngOnDestroy() will now destroy the children of the rootNgView.
NSLocationStrategy.ngOnDestroy() removes its outlets.

Fixes #1738

Related to #1728

…roy()

…iptRendererFactory is destroyed

…ively

Make sure outlets and currentOutlet are removed when the NSLocationStrategy is destroyed.

…stroy

When removing the children from a Label and one of these children is a FormattedString, the FormattedTextProperty throw this exception: "TypeError: Cannot read property 'textTransform' of undefined"

Without this the Page and componentView doesn't get the proper relation for then the page is destroyed.

…onent PageRouterOutlet.loadComponentInPage moves the componentView from its native parent, to the Page. Don't remove the componentView's children when it is removed from the native parent.

etabakov · 2019-02-25T16:44:45Z

@cla-bot check

cla-bot · 2019-02-25T16:44:49Z

The cla-bot has been summoned, and re-checked this pull request!

Just like with FormattedString this throws an exception

edusperoni · 2019-03-21T12:18:56Z

Upon reviewing #1767, I realized that view._modal is never dealt with.

This was usually not an issue, but when we navigate out (and this actually happens with livesync too), the modal will never close. Maybe the modal should also be destroyed on

nativescript-angular/nativescript-angular/view-util.ts

Line 256 in 5b4f8ff

private removeFromVisualTree(parent: NgView, child: NgView) {

Or maybe this should be handled by tns-core-modules on _removeView:

https://github.com/NativeScript/NativeScript/blob/3c2c1d9b69eac347474241af9303232043125d37/tns-core-modules/ui/core/view-base/view-base.ts#L614

m-abs · 2019-03-22T22:55:37Z

I wanted to checkout the behavior, unfortunately I have trouble running ng-sample.

But I'm thinking ViewUtils.removeFromVisualTree should check for modal and close it.

m-abs · 2019-04-16T15:05:54Z

Hi @edusperoni,

Do you have an example where view.modal isn't dealt with?

I have been trying to replicate the problem without any success.

EDIT:
I can easily add code to handle it in ViewUtils.removeFromVisualTree, but I since I can't reproduce the problem I'm not sure it is won't cause problems.

edusperoni · 2019-04-16T17:14:28Z

I think this is mainly an issue when livesyncing. If you do:

Page 1 -> Page 2 -> open Modal

and livesync, it'll go back to page 1 and not close the modal. But I don't think it's something that should be considered in this PR

vakrilov · 2019-05-07T11:23:37Z

nativescript-angular/router/page-router-outlet.ts

+
+        if (this.isActivated) {
+            const c = this.activated.instance;
+            this.activated.hostView.detach();


Probably no need to call detach on component that will be destroyed anyway

vakrilov · 2019-05-07T12:34:49Z

Hey there @m-abs
Looks like some really valid concerns are addressed in this PR.

The only change I didn't completely understand is ViewUtil.removeChild(parent, child) now removes children recursively to clear references. My thinking is that if you want to remove a large portion of the UI tree you should be able to do by just removing the root of the subtree. Why is there a need to removing each child recursively? There might be performance implication if there are many children or deeper UI tree. In the same time

m-abs · 2019-05-07T15:46:25Z

@vakrilov

The only change I didn't completely understand is ViewUtil.removeChild(parent, child) now removes children recursively to clear references.

That is unfortunately needed to clear up all references to firstChild, lastChild and nextSibling from the UI tree below the view being removed.
These references make it hard for the GC to clean om the disposed Views even after the native views have been destroyed.

It might help if I give an example.

If we have a UI-tree like this:

View1
  View1_1
    View1_1_1
      View1_1_1_1
      View1_1_1_2
    View1_1.2
    View1_1_3
  View1_2
  View1_3
  View1_4

This gives us these references:

View1.firstChild = View1_1
View1.lastChild = View1_4
View1_1.nextSibling = View1_2
View1_2.nextSibling = View1_3
View1_3.nextSibling = View1_4
View1_1.firstChild = View1_1_1
View1_1.lastChild = View1_1.3
View1_1_1.firstChild = View1_1_1_1
View1_1_1.lastChild = View1_1_1_2
View1_1_1_1.nextSibling = View1_1_1_2
View1_1_1.nextSibling = View1_1_2
View1_1_3.nextSibling = View1_1_3

Removing View1_1 will give us these:

View1.firstChild = View1_1
View1.lastChild = View1_4
View1_2.nextSibling = View1_3
View1_3.nextSibling = View1_4

View1_1_1.firstChild = View1_1_1_1
View1_1_1.lastChild = View1_1_1_2
View1_1_1_1.nextSibling = View1_1_1_2

View1_1_1.nextSibling = View1_1_2
View1_1_3.nextSibling = View1_1_3

(To simplify the example I left out the parentNode/parent references created by tns-core-modules)

edusperoni · 2019-05-07T17:11:18Z

Shouldn't only the parent and siblings be updated then?

View1.firstChild = View1_2 // checked and updated
View1.lastChild = View1_4 // checked and remained the same
View1_2.nextSibling = View1_3 // checked and remained the same
View1_3.nextSibling = View1_4 // checked and remained the same

View1_1_1.firstChild = View1_1_1_1
View1_1_1.lastChild = View1_1_1_2
View1_1_1_1.nextSibling = View1_1_1_2

View1_1_1.nextSibling = View1_1_2
View1_1_2.nextSibling = View1_1_3 // this would be updated if View1_1_3 were removed

Since there is no more reference to View1_1_1, the whole tree should be GCed.

edusperoni · 2019-05-07T19:19:36Z

I've added a simple example to illustrate this behavior: https://stackblitz.com/edit/typescript-pbquxj

How to check garbage collection:

Open Chrome on https://typescript-pbquxj.stackblitz.io
open dev tools and go into "Performance" (DO NOT USE STACKBLITZ'S CONSOLE)
Type in the console document.weakSet, all views will be there
after 5s a view will disappear
document.weakSet still shows all views
CLEAR THE CONSOLE (this is important, as chrome will keep the reference to the elements inside the WeakSet)
Press the Trash can (Collect garabage) in the performance tab a few times
check that the views vanished from document.weakSet

The same exercise could be done in the NativeScript runtime with WeakRef, but I believe it'll behave the same.

m-abs · 2019-05-08T13:35:28Z

Back when I started this PR, we had serious memory issues. It ended up delaying a release.

My stress test project as mentioned here could very easily crash. It would take about 1300 iterations or ~15 mins.

The behavior between [email protected] + [email protected] and [email protected] + [email protected] have improved a lot.

The stress test app no longer crash due to low memory, but it does end up using too much memory after many iterations.

I've been testing on Android using the Chrome Debugger to take heap snapshots manually. (I wish, I knew a better way to do it).

My reasoning behind removing the UI-tree recursively like that, was to make it easier for the Views to be GCed.

My guess is/was that are circular references in the sub-tree that makes it more difficult than needed for the GC.

Originally I just removed the firstChild, lastChild and nextSibling-references recursively in

nativescript-angular/nativescript-angular/view-util.ts

Line 188 in 2a64e87

private removeFromQueue(parent: NgView, child: NgView) {

, but I was afraid of causing some inconsistencies in the UI-tree.

I also tried making those properties into WeakRefs in

nativescript-angular/nativescript-angular/view-util.ts

Line 321 in 2a64e87

private setNgViewExtensions(view: View, name: string): NgView {

but it was too hacky for my taste.

@edusperoni
You're right with the latest tns-core-modules and tns-android.
The views are eventually GCed.

But GC takes longer, without recursively removing the child views.

I've run three stress tests with 1000 iterations.
There is a forced GC per 25 iteration.
Cold start each time.

1st test is with [email protected]:
Total time: 170442ms
Time spend on GC: 59443ms
Average GC time: 1486ms

2nd test is my PR without recursively removing children:
Total time: 171177ms
Time spend on GC: 59718ms
Average GC time: 1493ms

3rd test is with my PR as-is:
Total time: 165104ms
Time spend on GC: 52314ms
Average GC time: 1307ms

~7000ms less time spend on GC, if I remove the children recursively.

vakrilov · 2019-05-09T06:46:34Z

Indeed! The recursive remove does produce better results. I have also tried the same test with *ngFor instead of ListView and the results were consistently better.

Note: Adding "markingMode": "none" to the test project decreased the GC time from ~1600ms to ~65ms, which is x25 fold improvement. So I definitely recommend you to use it. In this setup the recursive update also performed slightly better.

That said - I'm sold on keeping the recursive remove. I have some other minor suggestions and we can merge this.

nativescript-angular/view-util.ts

m-abs · 2019-05-09T08:32:07Z

Note: Adding "markingMode": "none" to the test project decreased the GC time from ~1600ms to ~65ms, which is x25 fold improvement. So I definitely recommend you to use it. In this setup the recursive update also performed slightly better.

I tried "markingMode": "none" before and just now, it causes my test app to crash after a few iterations with:

05-09 10:29:01.224 19269 19269 W System.err: com.tns.NativeScriptException: Attempt to use cleared object reference id=9224
05-09 10:29:01.224 19269 19269 W System.err:    at com.tns.Runtime.getJavaObjectByID(Runtime.java:1006)
05-09 10:29:01.224 19269 19269 W System.err:    at com.tns.Runtime.callJSMethodNative(Native Method)
05-09 10:29:01.224 19269 19269 W System.err:    at com.tns.Runtime.dispatchCallJSMethodNative(Runtime.java:1203)
05-09 10:29:01.224 19269 19269 W System.err:    at com.tns.Runtime.callJSMethodImpl(Runtime.java:1083)
05-09 10:29:01.224 19269 19269 W System.err:    at com.tns.Runtime.callJSMethod(Runtime.java:1070)
05-09 10:29:01.224 19269 19269 W System.err:    at com.tns.Runtime.callJSMethod(Runtime.java:1050)
05-09 10:29:01.224 19269 19269 W System.err:    at com.tns.Runtime.callJSMethod(Runtime.java:1042)
05-09 10:29:01.224 19269 19269 W System.err:    at com.tns.gen.android.widget.BaseAdapter_view_197_32_ListViewAdapter.getView(BaseAdapter_view_197_32_ListViewAdapter.java:40)
...

But that belongs in another issue :)

That said - I'm sold on keeping the recursive remove. I have some other minor suggestions and we can merge this.

Cool :)

vakrilov

Looks great!

vakrilov · 2019-05-09T11:48:22Z

Thanks @m-abs
We will try to include this in a patch release next week along with NativeScript 5.4 release

SvetoslavTsenov · 2019-05-09T14:36:12Z

test ios@rc

SvetoslavTsenov · 2019-05-09T15:09:36Z

test ios@rc

SvetoslavTsenov · 2019-05-09T19:33:20Z

test ios#rc

m-abs added 6 commits February 18, 2019 17:00

fix(TemplatedItemsComponent): remove templates in ngOnDestroy()

8f70bd0

fix(PageRouterOutlet): Destroy the activated componentRef on ngOnDest…

e3d494f

…roy()

fix(NativeScriptRendererFactory): Empty the rootNgView when NativeScr…

fa1d69b

…iptRendererFactory is destroyed

fix(ViewUtil): ViewUtil.removeChild() doesn't remove the child recurs…

8c81711

…ively

fix(NSLocationStrategy): remove outlets in ngOnDestroy()

9d479a9

Make sure outlets and currentOutlet are removed when the NSLocationStrategy is destroyed.

fix: PageRouterOutlet.ngOnDestroy logged as NSLocationStrategy.ngOnDe…

cf4eba4

…stroy

ghost added the ♥ community PR label Feb 18, 2019

m-abs and others added 6 commits February 19, 2019 15:41

fix(ViewUtil): Removing FormattedString throw

493d062

When removing the children from a Label and one of these children is a FormattedString, the FormattedTextProperty throw this exception: "TypeError: Cannot read property 'textTransform' of undefined"

fix: use ViewUtils to add Component to Page

616c4c2

Without this the Page and componentView doesn't get the proper relation for then the page is destroyed.

fix: Remove all the children from the AppHostView

3bf1b8a

Merge branch 'master' into fix/ns-views-not-cleaned-on-removal

27baf95

fix: ViewUtil.removeFromVisualTree logged under wrong name

e74afc2

fix(PageRouterOutlet): Don't remove grandchild views when moving comp…

b9b55b6

…onent PageRouterOutlet.loadComponentInPage moves the componentView from its native parent, to the Page. Don't remove the componentView's children when it is removed from the native parent.

cla-bot bot added the cla: yes label Feb 25, 2019

m-abs added 2 commits March 6, 2019 11:48

Merge branch 'master' into fix/ns-views-not-cleaned-on-removal

596daec

fix: ViewUtils tried to remote Span elements from Labels

5b4f8ff

Just like with FormattedString this throws an exception

Merge branch 'master' into fix/ns-views-not-cleaned-on-removal

17ecfb5

m-abs added 2 commits April 5, 2019 12:17

Merge branch 'master' into fix/ns-views-not-cleaned-on-removal

1fb3a63

Merge branch 'master' into fix/ns-views-not-cleaned-on-removal

b049e6c

ghost added the new PR label Apr 15, 2019

Merge branch 'master' into fix/ns-views-not-cleaned-on-removal

9fb6bbb

ghost assigned elena-p Apr 19, 2019

ghost added in progress and removed ♥ community PR labels Apr 19, 2019

Merge branch 'master' into fix/ns-views-not-cleaned-on-removal

2a64e87

ghost removed the in progress label Apr 24, 2019

NativeScript deleted a comment from SvetoslavTsenov May 3, 2019

VladimirAmiorkov previously approved these changes May 3, 2019

View reviewed changes

VladimirAmiorkov requested a review from vakrilov May 3, 2019 14:30

vakrilov reviewed May 7, 2019

View reviewed changes

Merge branch 'master' into fix/ns-views-not-cleaned-on-removal

4e32e89

vakrilov suggested changes May 9, 2019

View reviewed changes

nativescript-angular/view-util.ts Outdated Show resolved Hide resolved

nativescript-angular/view-util.ts Outdated Show resolved Hide resolved

chor: skip detached elements for removeFromVisualTree

ee086c0

m-abs dismissed VladimirAmiorkov’s stale review via ee086c0 May 9, 2019 08:24

vakrilov approved these changes May 9, 2019

View reviewed changes

Merge branch 'master' into fix/ns-views-not-cleaned-on-removal

b9f2568

ghost assigned SvetoslavTsenov May 9, 2019

ghost added in progress and removed ♥ community PR labels May 9, 2019

vakrilov merged commit 59a1cde into NativeScript:master May 10, 2019

ghost removed the in progress label May 10, 2019

dtopuzov mentioned this pull request May 12, 2019

livesync broken after PR #1740 #1818

Closed

vakrilov mentioned this pull request May 13, 2019

fix: revert recursive remove -causing crashes #1819

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/ns views not cleaned on removal #1740

Fix/ns views not cleaned on removal #1740

m-abs commented Feb 18, 2019 •

edited

Loading

etabakov commented Feb 25, 2019

cla-bot bot commented Feb 25, 2019

edusperoni commented Mar 21, 2019

m-abs commented Mar 22, 2019 •

edited

Loading

m-abs commented Apr 16, 2019 •

edited

Loading

edusperoni commented Apr 16, 2019

vakrilov May 7, 2019

vakrilov commented May 7, 2019

m-abs commented May 7, 2019 •

edited

Loading

edusperoni commented May 7, 2019

edusperoni commented May 7, 2019

m-abs commented May 8, 2019 •

edited

Loading

vakrilov commented May 9, 2019

m-abs commented May 9, 2019 •

edited

Loading

vakrilov left a comment

vakrilov commented May 9, 2019

SvetoslavTsenov commented May 9, 2019

SvetoslavTsenov commented May 9, 2019

SvetoslavTsenov commented May 9, 2019

Fix/ns views not cleaned on removal #1740

Fix/ns views not cleaned on removal #1740

Conversation

m-abs commented Feb 18, 2019 • edited Loading

PR Checklist

What is the current behavior?

What is the new behavior?

etabakov commented Feb 25, 2019

cla-bot bot commented Feb 25, 2019

edusperoni commented Mar 21, 2019

m-abs commented Mar 22, 2019 • edited Loading

m-abs commented Apr 16, 2019 • edited Loading

edusperoni commented Apr 16, 2019

vakrilov May 7, 2019

Choose a reason for hiding this comment

vakrilov commented May 7, 2019

m-abs commented May 7, 2019 • edited Loading

edusperoni commented May 7, 2019

edusperoni commented May 7, 2019

m-abs commented May 8, 2019 • edited Loading

vakrilov commented May 9, 2019

m-abs commented May 9, 2019 • edited Loading

vakrilov left a comment

Choose a reason for hiding this comment

vakrilov commented May 9, 2019

SvetoslavTsenov commented May 9, 2019

SvetoslavTsenov commented May 9, 2019

SvetoslavTsenov commented May 9, 2019

m-abs commented Feb 18, 2019 •

edited

Loading

m-abs commented Mar 22, 2019 •

edited

Loading

m-abs commented Apr 16, 2019 •

edited

Loading

m-abs commented May 7, 2019 •

edited

Loading

m-abs commented May 8, 2019 •

edited

Loading

m-abs commented May 9, 2019 •

edited

Loading